Add poetry to image spec #3025

thomasjpfan · 2024-12-30T19:30:34Z

Tracking issue

Why are the changes needed?

This allows poetry uses to create a image spec with their dependencies. This PR treats poetry as a "dependency manager". This means that their local project will not be installed. If we see that poetry users want to install their local package, then we can extend this feature with a follow up PR.

What changes were proposed in this pull request?

This PR adds a requirements="poetry.lock" that creates an ImageSpec with poetry.

How was this patch tested?

I created a repo with poetry init with this workflow:

from flytekit import task, ImageSpec
import numpy as np
from random import choice
from string import ascii_letters


image = ImageSpec(
    name="numpy-poetry",
    requirements="poetry.lock",
    registry="localhost:30000",
    env={"A": "".join(choice(ascii_letters) for _ in range(20))},
)


@task(container_image=image)
def add_task() -> np.ndarray:
    x = np.ones(10)
    return x + 10

and ran pyflyte run --remote workflows/main.py.

Summary by Bito

This PR implements Poetry support in ImageSpec with lock file handling capabilities and Docker build integration. It enhances array node capabilities with ReferenceTask support and improves file handling for remote access. The implementation includes comprehensive error handling across components, particularly in file operations and logging systems. Changes include expanded testing infrastructure with improved variable naming and VSCode integration enhancements.

Unit tests added: True

Estimated effort to review (1-5, lower is better): 5

Signed-off-by: Thomas J. Fan <thomasjpfan@gmail.com>

flyte-bot · 2024-12-30T19:30:49Z

Code Review Agent Run #73368b

Actionable Suggestions - 5

tests/flytekit/unit/core/image_spec/test_default_builder.py - 2
- Consider renaming shadowed lock_file variable · Line 281-281
- Consider consolidating duplicate pytest parametrize decorator · Line 275-275
flytekit/image_spec/default_builder.py - 3
- Consider single $ for PATH variable · Line 58-58
- Consider moving poetry arg to template · Line 196-197
- Consider validating pyproject.toml existence · Line 216-217

Review Details

Files reviewed - 2 · Commit Range: 0813ae8..0813ae8
- flytekit/image_spec/default_builder.py
- tests/flytekit/unit/core/image_spec/test_default_builder.py
Files skipped - 0
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful

AI Code Review powered by

flyte-bot · 2024-12-30T19:35:03Z

Changelist by Bito

This pull request implements the following key changes.

Key Change	Files Impacted
Feature Improvement - Poetry Support for ImageSpec	- `default_builder.py` - Added Poetry lock file support and template for Docker builds - `test_default_builder.py` - Added test cases for Poetry lock file handling and error scenarios

flyte-bot · 2024-12-30T19:35:05Z

tests/flytekit/unit/core/image_spec/test_default_builder.py

    run_mock = Mock()
    monkeypatch.setattr("flytekit.image_spec.default_builder.run", run_mock)

-    uv_lock_file = tmp_path / "uv.lock"
-    uv_lock_file.write_text("this is a lock file")
+    lock_file = tmp_path / lock_file


Consider renaming shadowed lock_file variable

The variable name lock_file is being redefined in the function scope, shadowing the parameter with the same name. This could lead to confusion and potential bugs.

Code suggestion

Check the AI-generated fix before applying

Suggested change

lock_file = tmp_path / lock_file

lock_file_path = tmp_path / lock_file

Code Review Run #73368b

Is this a valid issue, or was it incorrectly flagged by the Agent?

it was incorrectly flagged

flyte-bot · 2024-12-30T19:35:06Z

tests/flytekit/unit/core/image_spec/test_default_builder.py

        builder.build_image(image_spec)


+@pytest.mark.parametrize("lock_file", ["uv.lock", "poetry.lock"])


Consider consolidating duplicate pytest parametrize decorator

Consider consolidating the duplicate @pytest.mark.parametrize decorator with the one at line 254 since both test functions use the same parameters.

Code suggestion

Check the AI-generated fix before applying

@@ -254,7 +254,6 @@ @pytest.mark.parametrize("lock_file", ["uv.lock", "poetry.lock"]) @pytest.mark.filterwarnings("ignore::UserWarning") def test_lock_errors_no_pyproject_toml(monkeypatch, tmp_path, lock_file): @@ -274,7 +274,6 @@ -@pytest.mark.parametrize("lock_file", ["uv.lock", "poetry.lock"]) @pytest.mark.filterwarnings("ignore::UserWarning") def test_uv_lock_error_no_packages(monkeypatch, tmp_path, lock_file):

Code Review Run #73368b

Is this a valid issue, or was it incorrectly flagged by the Agent?

it was incorrectly flagged

flytekit/image_spec/default_builder.py

Signed-off-by: Thomas J. Fan <thomasjpfan@gmail.com>

flyte-bot · 2024-12-30T20:41:17Z

Code Review Agent Run #edb466

Actionable Suggestions - 0

Review Details

Files reviewed - 1 · Commit Range: 0813ae8..b32a4df
- tests/flytekit/unit/core/image_spec/test_default_builder.py
Files skipped - 0
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful

AI Code Review powered by

Signed-off-by: Thomas J. Fan <thomasjpfan@gmail.com>

flyte-bot · 2025-01-03T18:18:59Z

Code Review Agent Run #9afb0f

Actionable Suggestions - 0

Additional Suggestions - 7

flytekit/clis/sdk_in_container/run.py - 1
- Consider consolidating JSON serialization logic · Line 478-486
flytekit/core/array_node.py - 1
- Consider adding data mode validation check · Line 140-141
tests/flytekit/unit/cli/pyflyte/test_run.py - 2
- Consider checking exact message occurrences · Line 480-480
- Consider more precise output assertions · Line 853-854
tests/flytekit/integration/remote/test_remote.py - 2
- Consider using unittest.mock instead of mock · Line 17-17
- Consider adding cleanup error handling · Line 832-835
tests/flytekit/unit/bin/test_python_entrypoint.py - 1
- Consider consolidating repeated timestamp initialization pattern · Line 529-540

Review Details

Files reviewed - 28 · Commit Range: b32a4df..fa746c8
- dev-requirements.txt
- flytekit/clis/sdk_in_container/run.py
- flytekit/core/array_node.py
- flytekit/core/array_node_map_task.py
- flytekit/core/context_manager.py
- flytekit/core/local_cache.py
- flytekit/core/tracker.py
- flytekit/core/workflow.py
- flytekit/extend/backend/agent_service.py
- flytekit/extend/backend/base_agent.py
- flytekit/interactive/vscode_lib/decorator.py
- flytekit/loggers.py
- flytekit/models/core/workflow.py
- flytekit/models/literals.py
- flytekit/tools/translator.py
- flytekit/types/directory/types.py
- flytekit/types/file/file.py
- plugins/flytekit-airflow/setup.py
- plugins/flytekit-flyteinteractive/tests/test_flyteinteractive_vscode.py
- pyproject.toml
- tests/flytekit/integration/remote/test_remote.py
- tests/flytekit/integration/remote/utils.py
- tests/flytekit/integration/remote/workflows/basic/flytefile.py
- tests/flytekit/integration/remote/workflows/basic/pydantic_wf.py
- tests/flytekit/unit/bin/test_python_entrypoint.py
- tests/flytekit/unit/cli/pyflyte/test_run.py
- tests/flytekit/unit/core/test_local_cache.py
- tests/flytekit/unit/types/directory/test_listdir.py
Files skipped - 1
- .github/workflows/pythonpublish.yml - Reason: Filter setting
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful

AI Code Review powered by

* Store protos in local cache (#3022) * Store proto obj instead of model Literal in local cache Signed-off-by: Eduardo Apolinario <eapolinario@users.noreply.github.com> * Remove unused file Signed-off-by: Eduardo Apolinario <eapolinario@users.noreply.github.com> --------- Signed-off-by: Eduardo Apolinario <eapolinario@users.noreply.github.com> Co-authored-by: Eduardo Apolinario <eapolinario@users.noreply.github.com> * Bump aiohttp from 3.9.5 to 3.10.11 (#3018) Bumps [aiohttp](https://github.com/aio-libs/aiohttp) from 3.9.5 to 3.10.11. - [Release notes](https://github.com/aio-libs/aiohttp/releases) - [Changelog](https://github.com/aio-libs/aiohttp/blob/master/CHANGES.rst) - [Commits](aio-libs/aiohttp@v3.9.5...v3.10.11) --- updated-dependencies: - dependency-name: aiohttp dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Fix bug in FlyteDirectory.listdir on local files (#2926) * Fix issue in FlyteDirectory.listdir Fixes flyteorg/flyte#6005 Signed-off-by: Pim de Haan <pim@cusp.ai> * Added test Signed-off-by: Pim de Haan <pim@cusp.ai> * Run make lint Signed-off-by: Eduardo Apolinario <eapolinario@users.noreply.github.com> --------- Signed-off-by: Pim de Haan <pim@cusp.ai> Signed-off-by: Eduardo Apolinario <eapolinario@users.noreply.github.com> Co-authored-by: Eduardo Apolinario <eapolinario@users.noreply.github.com> * Fix unit tests in airflow plugin (#3024) Signed-off-by: Kevin Su <pingsutw@apache.org> * fix: Fix resource meta typos for async agent (#3023) Signed-off-by: JiaWei Jiang <waynechuang97@gmail.com> * fix: format commands output (#3026) * Fix pydantic basemodel default input (#3013) * Fix pydantic default input Signed-off-by: Future-Outlier <eric901201@gmail.com> * add pydantic integration test Signed-off-by: Future-Outlier <eric901201@gmail.com> * Use duck typing by Thomas's advice Signed-off-by: Future-Outlier <eric901201@gmail.com> Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com> * lint Signed-off-by: Future-Outlier <eric901201@gmail.com> --------- Signed-off-by: Future-Outlier <eric901201@gmail.com> Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com> * [BUG] Open FlyteFile from remote path (#2991) * fix: Open FlyteFile from remote path Signed-off-by: JiaWei Jiang <waynechuang97@gmail.com> * Add integration test Signed-off-by: JiaWei Jiang <waynechuang97@gmail.com> * refactor: Use ctx as param instead of recreation Signed-off-by: JiaWei Jiang <waynechuang97@gmail.com> * refactor: Clean test logic 1. Remove redundant prints 2. Use `mock.patch.dict` to setup `os.environ` for the current test fn * Avoid contaminating other tests running in the same process Signed-off-by: JiaWei Jiang <waynechuang97@gmail.com> * refactor: Setup local path and downloader in constructor Signed-off-by: JiaWei Jiang <waynechuang97@gmail.com> * refactor: Move SimpleFileTransfer to an utility file Signed-off-by: JiaWei Jiang <waynechuang97@gmail.com> * Remove redundant env var setup Please refer to #3001 Signed-off-by: JiaWei Jiang <waynechuang97@gmail.com> * test: Add another ff use case Create ff in one task pod and read it in another task pod. Signed-off-by: JiaWei Jiang <waynechuang97@gmail.com> --------- Signed-off-by: JiaWei Jiang <waynechuang97@gmail.com> * vllm inference plugin (#2967) * vllm inference plugin Signed-off-by: Daniel Sola <daniel.sola@union.ai> * fixed default value Signed-off-by: Daniel Sola <daniel.sola@union.ai> --------- Signed-off-by: Daniel Sola <daniel.sola@union.ai> * Add poetry to image spec (#3025) * Add poetry to image spec Signed-off-by: Thomas J. Fan <thomasjpfan@gmail.com> * Add stricter check Signed-off-by: Thomas J. Fan <thomasjpfan@gmail.com> --------- Signed-off-by: Thomas J. Fan <thomasjpfan@gmail.com> * [test] Add integration test for accessing sd sttr in dc (#2969) * test: Add integration test for attr access of sd Signed-off-by: JiaWei Jiang <waynechuang97@gmail.com> * Correct file path Signed-off-by: JiaWei Jiang <waynechuang97@gmail.com> * test: Support interaction with minio s3 bucket 1. Upload a local parquet file to minio s3 bucket 2. Access StructuredDataset attr from a dataclass 3. Open StructuredDataset from a remote path Signed-off-by: JiaWei Jiang <waynechuang97@gmail.com> * Delete an unmerged integration test Signed-off-by: JiaWei Jiang <waynechuang97@gmail.com> * Try imagespec with commit sha of corresponding fix Signed-off-by: JiaWei Jiang <waynechuang97@gmail.com> * Remove redundant test Signed-off-by: JiaWei Jiang <waynechuang97@gmail.com> * Remove default_factory and create sd dc from input uri Signed-off-by: JiaWei Jiang <waynechuang97@gmail.com> * refactor: Clean test logic 1. Remove redundant prints 2. Use `mock.patch.dict` to setup `os.environ` for the current test fn * Avoid contaminating other tests running in the same process Signed-off-by: JiaWei Jiang <waynechuang97@gmail.com> * Remove redundant minio env var setup and add test comments Signed-off-by: JiaWei Jiang <waynechuang97@gmail.com> * Support uploading tmp pqt file Signed-off-by: JiaWei Jiang <waynechuang97@gmail.com> * Udpate deprecated module Signed-off-by: JiaWei Jiang <waynechuang97@gmail.com> * Remove redundant and unused imports Signed-off-by: JiaWei Jiang <waynechuang97@gmail.com> --------- Signed-off-by: JiaWei Jiang <waynechuang97@gmail.com> --------- Signed-off-by: Eduardo Apolinario <eapolinario@users.noreply.github.com> Signed-off-by: dependabot[bot] <support@github.com> Signed-off-by: Pim de Haan <pim@cusp.ai> Signed-off-by: Kevin Su <pingsutw@apache.org> Signed-off-by: JiaWei Jiang <waynechuang97@gmail.com> Signed-off-by: Future-Outlier <eric901201@gmail.com> Signed-off-by: Daniel Sola <daniel.sola@union.ai> Signed-off-by: Thomas J. Fan <thomasjpfan@gmail.com> Co-authored-by: Eduardo Apolinario <653394+eapolinario@users.noreply.github.com> Co-authored-by: Eduardo Apolinario <eapolinario@users.noreply.github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Pim de Haan <pimdehaan@gmail.com> Co-authored-by: Kevin Su <pingsutw@apache.org> Co-authored-by: 江家瑋 <36886416+JiangJiaWei1103@users.noreply.github.com> Co-authored-by: V <0426vincent@gmail.com> Co-authored-by: Han-Ru Chen (Future-Outlier) <eric901201@gmail.com> Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com> Co-authored-by: Daniel Sola <40698988+dansola@users.noreply.github.com>

* Add poetry to image spec Signed-off-by: Thomas J. Fan <thomasjpfan@gmail.com> * Add stricter check Signed-off-by: Thomas J. Fan <thomasjpfan@gmail.com> --------- Signed-off-by: Thomas J. Fan <thomasjpfan@gmail.com> Signed-off-by: Shuying Liang <shuying.liang@gmail.com>

Add poetry to image spec

0813ae8

Signed-off-by: Thomas J. Fan <thomasjpfan@gmail.com>

thomasjpfan requested review from wild-endeavor, kumare3, eapolinario, pingsutw, cosmicBboy, samhita-alla and Future-Outlier as code owners December 30, 2024 19:30

flyte-bot reviewed Dec 30, 2024

View reviewed changes

flytekit/image_spec/default_builder.py Show resolved Hide resolved

flyte-bot reviewed Dec 30, 2024

View reviewed changes

flytekit/image_spec/default_builder.py Show resolved Hide resolved

flyte-bot reviewed Dec 30, 2024

View reviewed changes

flytekit/image_spec/default_builder.py Show resolved Hide resolved

Add stricter check

b32a4df

Signed-off-by: Thomas J. Fan <thomasjpfan@gmail.com>

wild-endeavor approved these changes Jan 2, 2025

View reviewed changes

Merge remote-tracking branch 'upstream/master' into flytekit-2

fa746c8

Signed-off-by: Thomas J. Fan <thomasjpfan@gmail.com>

thomasjpfan merged commit 87ac7f7 into flyteorg:master Jan 6, 2025
102 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add poetry to image spec #3025

Add poetry to image spec #3025

thomasjpfan commented Dec 30, 2024 •

edited by flyte-bot

Loading

flyte-bot commented Dec 30, 2024 •

edited

Loading

Code Review Agent Run #73368b

flyte-bot commented Dec 30, 2024 •

edited

Loading

Changelist by Bito

flyte-bot Dec 30, 2024

flyte-bot Dec 30, 2024

flyte-bot commented Dec 30, 2024 •

edited

Loading

Code Review Agent Run #edb466

flyte-bot commented Jan 3, 2025 •

edited

Loading

Code Review Agent Run #9afb0f

	lock_file = tmp_path / lock_file
	lock_file_path = tmp_path / lock_file

		builder.build_image(image_spec)


		@pytest.mark.parametrize("lock_file", ["uv.lock", "poetry.lock"])

Add poetry to image spec #3025

Add poetry to image spec #3025

Conversation

thomasjpfan commented Dec 30, 2024 • edited by flyte-bot Loading

Tracking issue

Why are the changes needed?

What changes were proposed in this pull request?

How was this patch tested?

Summary by Bito

flyte-bot commented Dec 30, 2024 • edited Loading

Code Review Agent Run #73368b

flyte-bot commented Dec 30, 2024 • edited Loading

Changelist by Bito

flyte-bot Dec 30, 2024

Choose a reason for hiding this comment

flyte-bot Dec 30, 2024

Choose a reason for hiding this comment

flyte-bot commented Dec 30, 2024 • edited Loading

Code Review Agent Run #edb466

flyte-bot commented Jan 3, 2025 • edited Loading

Code Review Agent Run #9afb0f

thomasjpfan commented Dec 30, 2024 •

edited by flyte-bot

Loading

flyte-bot commented Dec 30, 2024 •

edited

Loading

flyte-bot commented Dec 30, 2024 •

edited

Loading

flyte-bot commented Dec 30, 2024 •

edited

Loading

flyte-bot commented Jan 3, 2025 •

edited

Loading